Skip to content

YK Myhomework#12

Open
Yurii-Kulinich wants to merge 3 commits intodmdev2020:homeworkfrom
Yurii-Kulinich:myhomework
Open

YK Myhomework#12
Yurii-Kulinich wants to merge 3 commits intodmdev2020:homeworkfrom
Yurii-Kulinich:myhomework

Conversation

@Yurii-Kulinich
Copy link
Copy Markdown

No description provided.

.toList();
assertThat(ids).contains(subscriptionFirst.getId(), subscriptionSecond.getId(),
subscriptionThird.getId());
}
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Разделяй четко 3 секции в тесте, чтобы улучшить его читабильность: given/when/then

List<Integer> ids = result.stream()
.map(Subscription::getId)
.toList();
assertThat(ids).contains(subscriptionFirst.getId(), subscriptionSecond.getId(),
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

нет смысла в этом ассерте, когда есть assertEquals такой чуть выше

var result = subscriptionDao.delete(subscriptionFirst.getId());

assertTrue(result);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

не надо делать отступов в конце метода

void deleteWhenIdNotExistsThenReturnFalse() {
subscriptionDao.insert(getSubscription("first"));

var result = subscriptionDao.delete(1234);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

нужно выбирать более нереалистичное значение id
1234 легко достичь в тестах, что приведет к его flaky

var subscriptionFirst = subscriptionDao.insert(getSubscription("first").setUserId(userid));
var subscriptionSecond = subscriptionDao.insert(getSubscription("second").setUserId(userid));

List<Subscription> result = subscriptionDao.findByUserId(userid);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для теста метода findByUserId нужна вставить несколько подписок с разным userId - иначе некорректный dataset. Его можно заменить на findAll, например, и он все еще будет проходить

private CreateSubscriptionValidator createSubscriptionValidator;

@Mock
private Clock clock;
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clock не мокают обычно. Этот класс используется сам по себе для тестов. Почитать документацию его


@Test
void whenCallingUpsertMethodWithInvalidDtoThenThrowValidationException() {
var dto = Mockito.mock(CreateSubscriptionDto.class);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

мокают обычно только зависимости в логике, а не данные. Данные используют максимально реалистичные

subscriptionService.cancel(any());

verify(subscriptionDao, times(1)).update(updatedSubscription);
verify(subscriptionDao, times(1)).findById(any());
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

times(1) - это по умолчанию. Нет смысла указывать дефолты явно


class PropertiesUtilTest {


Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 строки отступа достаточно между классом и его полями/методами

import org.mockito.junit.jupiter.MockitoExtension;

@ExtendWith(MockitoExtension.class)
class SubscriptionServiceTest {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

надо все методы покрыть было интеграционными тестами. SubscriptionService - это самый важный сервис!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants